fix: Also apply sort params to existing fields in a ListGuesser#629
fix: Also apply sort params to existing fields in a ListGuesser#629zolex wants to merge 1 commit intoapi-platform:mainfrom
Conversation
slax57
left a comment
slax57
left a comment
There was a problem hiding this comment.
Thanks for offering to contribute in any case!
| const fieldProps = { | ||
| ...field.props, | ||
| key: field.props.source + (orderField ? `-${orderField}` : ''), | ||
| sortBy: orderField, | ||
| sortable: !!orderField, | ||
| }; |
There was a problem hiding this comment.
I'm afraid this is overriding existing props on the field, e.g. if you had set sortable={false} then this is lost.
Besides, IMO this is not the react-admin way to do things.
As of late react-admin tends to avoid inspecting the children and overriding their props directly as this can lead to unexpected behaviors. Instead, I wonder if the best solution would rather be to update the getOverrideCode to include the sortBy and sortable props in the generated code, which would encourage users to keep those props once they move to providing their own children.
There was a problem hiding this comment.
I'm afraid this is overriding existing props on the field, e.g. if you had set
sortable={false}then this is lost.Besides, IMO this is not the react-admin way to do things. As of late react-admin tends to avoid inspecting the children and overriding their props directly as this can lead to unexpected behaviors. Instead, I wonder if the best solution would rather be to update the
getOverrideCodeto include thesortByandsortableprops in the generated code, which would encourage users to keep those props once they move to providing their own children.
thanks for the input, I will have a look.
2 participants
Also apply sort params to existing fields in a ListGuesser instead of only "empty" listguessers.